fix: bastion and machine reconcile defects - #4
Merged
Conversation
… and deduplicate repeated allowedCIDRs, each with a regression test
…th an envtest regression test
… repaired instead of aborting bastion reconciliation
… instead of hanging in Terminating
…leave it running while reporting itself disabled
Herbaert
added a commit
that referenced
this pull request
Aug 14, 2026
…h fix, disable-teardown and credentials-Secret finalize as fixed, three new open items (recreate guard has no terminal state, CIDR string comparison, status.ready gaps)
tuunit
requested changes
Aug 14, 2026
…written JSON body
…signing Status.Ready directly
tuunit
requested changes
Aug 17, 2026
…ec or status combination can leak them
…bility Signed-off-by: Jan Larwig <jan@larwig.com>
tuunit
approved these changes
Aug 18, 2026
Herbaert
added a commit
that referenced
this pull request
Aug 19, 2026
This branch had independently re-implemented the same bastion/machine reconcile fixes that PR #4 already merged into main, with different variable names and structure. Replace those 14 files 1:1 with main's version instead of maintaining a diverging parallel implementation. debug/ is untouched here; its file:line references are corrected next.
Herbaert
added a commit
that referenced
this pull request
Aug 19, 2026
… status.Ready assignments through ClusterScope.SetNotReady (per PR #4 review), and standardise speaking variable names/formatting across Go files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes seven defects in the bastion and machine reconcile paths, each with a
regression test. All were found by running the provider against real STACKIT
infrastructure and by the Copilot review on #1; none are introduced by the
refactor — they exist on
maintoo.What was broken
Silent resource leaks
ensureServerrecreated a server for machines that had already bootstrappedand joined. The replacement replays the original bootstrap data, so it either
never rejoins (different IP) or rejoins while
MachineandNodekeeppointing at the deleted server (same IP) — the latter reports a healthy 3/3
cluster while carrying a dangling provider ID. Now surfaces an error instead
of recreating.
enabled: false, wereboth gated on persisted status. A bastion whose status patch never landed was
therefore never removed — in the disable case while the condition reported
"bastion disabled" and port 22 stayed open. Both now follow intent; the
tag-based lookups make this safe.
Security
allowedCIDRsnever revoked the old rule: the rule set only evergrew, so narrowing the CIDR did not take access away from the previously
allowed range. Rules are now reconciled in both directions.
Reconcile correctness
CreateServerhad already included it, failing with 404 (no port yet) or 400 ("Duplicate
items in the list") and aborting
EnsureBastionbefore the public IP wasassigned. The attach is kept —
CreateServershort-circuits on an existingserver, making it the only path that repairs a detached group — but is now
idempotent.
Terminatingforever. A missing Secret can never be recovered from (itcommonly disappears first during namespace teardown), so deletion now
finalizes and emits a
CleanupSkippedwarning; invalid — i.e. fixable —credentials still block as before.
Status consistency
status.readybooleantruewhile theconditions said
False.Configuration
cluster-template-bastion.yamlhardcodedreplicas: 3, ignoringWORKER_MACHINE_COUNT.Testing
Every fix ships with a test at the cheapest level that can actually observe it:
cloudunit tests (httptest against the STACKIT API) for the client defects,envtest specs for the controller defects.
Each test was verified by reverting its fix and confirming the test fails at
the documented assertion — a test that passes for unrelated reasons proves
nothing. Coverage:
cloud39.1% → 56.3%,controller70.2% → 71.1%.The suite was also run end to end against real STACKIT infrastructure (cluster
lifecycle, providerID alignment, bastion), all green with no leaked resources.
Follow-ups not in this PR
failure, so without a
MachineHealthCheckthe Machine is never replaced;status.instanceStateandaddressesalso keep describing the deletedserver.
allowedCIDRsare compared as strings, so a non-canonical prefix the APIstores masked causes create/delete churn.
re-reconciles the cluster.
StackitCluster→Machinewatch matchesMachine.spec.clusterNameagainst the
StackitClustername, which only holds when both share a name.